ORCA: Fix use-after-free in flatten_join_alias_var_optimizer and incorrect HAVING decorrelation#1619
Closed
ORCA: Fix use-after-free in flatten_join_alias_var_optimizer and incorrect HAVING decorrelation#1619
Conversation
Guard pfree/list_free calls with pointer-equality checks to avoid freeing live nodes when flatten_join_alias_vars returns the same pointer unchanged (e.g., outer-reference Vars with varlevelsup > 0). The unconditional pfree(havingQual) freed the Var node, whose memory was later reused by palloc for a T_List. copyObjectImpl then copied the wrong node type into havingQual, causing ORCA to encounter an unexpected RangeTblEntry and fall back to the Postgres planner. Applies the same guard pattern to all six fields: targetList, returningList, havingQual, scatterClause, limitOffset, limitCount. Reported-in: #1618
Force correlated execution (SubPlan) for scalar subqueries with GROUP BY () and a correlated HAVING clause. Previously ORCA decorrelated such subqueries into Left Outer Join + COALESCE(count,0), which incorrectly returned 0 instead of NULL when the HAVING condition was false. Add FHasCorrelatedSelectAboveGbAgg() to detect the pattern where NormalizeHaving() has converted the HAVING clause into a CLogicalSelect with outer refs above a CLogicalGbAgg with empty grouping columns. When detected, set m_fCorrelatedExecution = true in Psd() to bypass the COALESCE decorrelation path. Update groupingsets_optimizer.out expected output to reflect the new ORCA SubPlan format instead of Postgres planner fallback. Reported-in: #1618
yjhjstz
force-pushed
the
yjhjstz/fix/orca-use-after-free-having-decorrelation
branch
from
69a2a53 to
9031693
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes two ORCA-related correctness issues: a use-after-free in query normalization (flatten_join_alias_var_optimizer) and an incorrect decorrelation for scalar subqueries of the form GROUP BY () HAVING <outer_ref>, which could produce 0 instead of NULL.
Changes:
- Guard
pfree()/list_free()inflatten_join_alias_var_optimizer()to avoid freeing nodes whenflatten_join_alias_vars()returns the same pointer. - Add ORCA detection logic intended to prevent decorrelation for correlated
GROUP BY () HAVING <outer_ref>scalar subqueries and avoid applyingCOALESCE(count, 0)in that case. - Update
groupingsets_optimizer.outexpected output to reflect GPORCA SubPlan execution.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/backend/optimizer/util/clauses.c | Prevents UAF by only freeing old pointers when flattening returns a different node/list. |
| src/backend/gporca/libgpopt/src/xforms/CSubqueryHandler.cpp | Adds pattern detection to avoid incorrect decorrelation/coalesce semantics for specific HAVING-over-empty-GbAgg cases. |
| src/test/regress/expected/groupingsets_optimizer.out | Updates expected EXPLAIN output to match the new GPORCA plan shape (SubPlan path). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+366
to
+376
| if (COperator::EopLogicalSelect == pexpr->Pop()->Eopid() && | ||
| pexpr->HasOuterRefs()) | ||
| { | ||
| // The Select has outer references somewhere in its subtree. | ||
| // Check whether they originate from the filter (child 1) rather | ||
| // than from the logical child (child 0). If the logical child has | ||
| // no outer refs but the Select as a whole does, the outer refs must | ||
| // come from the filter predicate — exactly the correlated-HAVING | ||
| // pattern we want to detect. | ||
| CExpression *pexprLogicalChild = (*pexpr)[0]; | ||
| if (!pexprLogicalChild->HasOuterRefs() && |
Comment on lines
+859
to
+870
| // When GROUP BY () has a correlated HAVING clause (now represented as a | ||
| // CLogicalSelect with outer refs sitting above the GbAgg), the subquery | ||
| // must return NULL — not 0 — when the HAVING condition is false. | ||
| // Applying COALESCE(count,0) would incorrectly convert that NULL to 0, | ||
| // so we skip the special count(*) semantics in that case. | ||
| BOOL fCorrelatedHavingAboveEmptyGby = | ||
| (fHasCountAggMatchingColumn && 0 == pgbAgg->Pdrgpcr()->Size() && | ||
| FHasCorrelatedSelectAboveGbAgg((*pexprSubquery)[0])); | ||
|
|
||
| if (fGeneratedByQuantified || | ||
| (fHasCountAggMatchingColumn && 0 == pgbAgg->Pdrgpcr()->Size())) | ||
| (fHasCountAggMatchingColumn && 0 == pgbAgg->Pdrgpcr()->Size() && | ||
| !fCorrelatedHavingAboveEmptyGby)) |
yjhjstz
deleted the
yjhjstz/fix/orca-use-after-free-having-decorrelation
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
flatten_join_alias_var_optimizerwhere unconditionalpfree/list_freefreed live nodes whenflatten_join_alias_varsreturned the same pointer unchangedGROUP BY () HAVING <outer_ref>subqueries, which returned0instead ofNULLwhen the HAVING condition was falseRoot Cause
use-after-free (
clauses.c):pfree(havingQual)freed thev.cVar node unconditionally. The freed memory was reused bypallocfor aT_List(nodeTag=596). WhencopyObjectImpllater readhavingQual, it copied the wrong node type, causing ORCA to encounter an unexpectedRangeTblEntryand fall back to the Postgres planner — which masked the second bug.ORCA decorrelation (
CSubqueryHandler.cpp): After fixing the use-after-free, ORCA no longer fell back and attempted to decorrelate the subquery. It convertedGROUP BY () HAVING <outer_ref>intoLeft Outer Join + COALESCE(count(*), 0), incorrectly returning0instead ofNULLwhen the HAVING condition was false.Fix
pfree/list_freecalls inflatten_join_alias_var_optimizerwith pointer-equality checksCLogicalSelectaboveCLogicalGbAgg[GROUP BY()]inPsd()and forcem_fCorrelatedExecution = trueto use SubPlan instead of decorrelationgroupingsets_optimizer.outexpected output for the new ORCA SubPlan formatTest plan
groupingsetstest passes with correct result (f | NULL,t | 9)Fixes #1618